Skip to content

Conversation

@sanjaysargam
Copy link
Member

Purpose / Description

  • Enable search functionality for preferences under Reviews and Previews tabs in Controls settings
  • Add automatic tab navigation when accessing tabbed preferences from search results
  • Handle duplicate preferences with distinct context labels

Fixes

How Has This Been Tested?

WhatsApp.Video.2025-11-03.at.11.17.51.PM.mp4

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@david-allison
Copy link
Member

A test fails

PrefsSearchBarTest > All indexed XML resIDs lead to the correct fragments on getFragmentFromXmlRes FAILED
    java.lang.AssertionError: com.ichi2.anki.debug:xml/preferences_reviewer_controls should match the preferenceResource of ControlsSettingsFragment
    Expected: <2132082712>
         but: was <2132082704>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at com.ichi2.anki.preferences.PrefsSearchBarTest.All indexed XML resIDs lead to the correct fragments on getFragmentFromXmlRes(PrefsSearchBarTest.kt:71)

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Nov 4, 2025
@sanjaysargam sanjaysargam added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Nov 5, 2025
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

  • 1 issue of accessibility/ease of use
  • 1 issue of maintainability

Let me know if you'd like more detail on either of the comments - I realise I didn't provide an explicit path to resolution

Comment on lines 144 to 147
delay(100)
fragment.selectTabForPreference(result.key)
delay(150)
(fragment as PreferenceFragmentCompat).let { result.highlight(it) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should handle animations being disabled

I don't know what the code for this would be, but this should handle users speeding up animations in the developer options of a phone (I strongly dislike long animations)


/**
* Determines if a preference key belongs to the previewer tab.
* Previewer preferences typically have "previewer_" prefix in their keys.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way we can ensure that this invariant holds in future?

Ideally a test/enum: this is brittle and people will break it

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Nov 5, 2025
@sanjaysargam
Copy link
Member Author

  • 1 issue of accessibility/ease of use
  • 1 issue of maintainability

Can you elaborate a bit more

@sanjaysargam sanjaysargam removed the Needs Author Reply Waiting for a reply from the original author label Nov 5, 2025
@david-allison david-allison added the Needs reviewer reply Waiting for a reply from another reviewer label Nov 5, 2025
@david-allison david-allison added this to the 2.23 release milestone Nov 5, 2025
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One 'safe display mode' issue

result.highlight(fragment as PreferenceFragmentCompat)
if (isTabSpecificPreference(result.resourceFile)) {
(fragment as? ControlsSettingsFragment)?.lifecycleScope?.launch {
val durationScale =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract to a helper property/method

1f // Default to normal speed if unable to read setting
}

if (durationScale > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't take 'safe display mode' into account

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm planning to use the same animation check pattern which is used in DeckPickerFloatingActionMenu.kt

private fun areSystemAnimationsEnabled(): Boolean {
        val animDuration: Float =
            Settings.Global.getFloat(
                context.contentResolver,
                Settings.Global.ANIMATOR_DURATION_SCALE,
                1f,
            )
        val animTransition: Float =
            Settings.Global.getFloat(
                context.contentResolver,
                Settings.Global.TRANSITION_ANIMATION_SCALE,
                1f,
            )
        val animWindow: Float =
            Settings.Global.getFloat(
                context.contentResolver,
                Settings.Global.WINDOW_ANIMATION_SCALE,
                1f,
            )
        return animDuration != 0f && animTransition != 0f && animWindow != 0f
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about AnkiActivity::animationDisabled which is an in-app setting

@david-allison david-allison added Needs Author Reply Waiting for a reply from the original author and removed Needs Review Needs reviewer reply Waiting for a reply from another reviewer labels Nov 7, 2025
@sanjaysargam sanjaysargam added Needs Review Needs reviewer reply Waiting for a reply from another reviewer and removed Needs Author Reply Waiting for a reply from the original author labels Nov 7, 2025
@lukstbit lukstbit removed the Needs reviewer reply Waiting for a reply from another reviewer label Nov 9, 2025
@BrayanDSO BrayanDSO self-requested a review November 14, 2025 12:35
Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more maintainable approach:

Subject: [PATCH] test(preferences): controls keys
---
Index: AnkiDroid/src/main/java/com/ichi2/anki/preferences/ControlsSettingsFragment.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/ControlsSettingsFragment.kt b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/ControlsSettingsFragment.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/ControlsSettingsFragment.kt	(revision a268ec898a0706d1802ea5d54576dc52a091504f)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/ControlsSettingsFragment.kt	(date 1763127894446)
@@ -66,12 +66,11 @@
      * This allows search navigation to automatically switch to the correct tab.
      */
     fun selectTabForPreference(key: String) {
-        val targetTabIndex =
-            if (previewerPreferenceKeys.contains(key)) {
-                ControlPreferenceScreen.PREVIEWER.ordinal
-            } else {
-                ControlPreferenceScreen.REVIEWER.ordinal
-            }
+        val targetTabIndex = actionToScreenMap[key]?.ordinal
+        if (targetTabIndex == null) {
+            Timber.w("Could not find the preference with the '%s' key", key)
+            return
+        }
 
         view?.post {
             val tabLayout =
@@ -182,7 +181,10 @@
     }
 
     companion object {
-        private val previewerPreferenceKeys = PreviewerAction.entries.map { it.preferenceKey }.toSet()
+        val actionToScreenMap: Map<String, ControlPreferenceScreen> =
+            ControlPreferenceScreen.entries.flatMap { screen ->
+                screen.getActions().map { action -> action.preferenceKey to screen }
+            }.toMap()
 
         val legacyStudyScreenSettings =
             listOf(

Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be easier to review if you put the Safe display mode changes in a separate commit.

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Nov 14, 2025
@sanjaysargam sanjaysargam force-pushed the control branch 2 times, most recently from d80448d to 49eb1da Compare November 18, 2025 11:49
@sanjaysargam sanjaysargam removed Needs Author Reply Waiting for a reply from the original author Has Conflicts labels Nov 19, 2025
Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Index: AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt	(revision 49eb1da070b446c9e4617dfab8a342fef8c81e39)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt	(date 1763544734216)
@@ -141,8 +141,8 @@
             addToBackStack(fragment.javaClass.name)
         }
 
-        if (isTabSpecificPreference(result.resourceFile)) {
-            (fragment as? ControlsSettingsFragment)?.lifecycleScope?.launch {
+        if (fragment is ControlsSettingsFragment) {
+            fragment.lifecycleScope.launch {
                 if (areSystemAnimationsEnabled(requireContext())) {
                     delay(100)
                     fragment.selectTabForPreference(result.key)
@@ -159,14 +159,6 @@
         }
     }
 
-    /**
-     * Checks if the given resource file represents a tab-specific preference
-     * that requires special tab navigation handling.
-     */
-    private fun isTabSpecificPreference(
-        @XmlRes file: Int,
-    ) = ControlPreferenceScreen.entries.any { it.xmlRes == file }
-
     private fun setupBackCallbacks() {
         requireActivity().onBackPressedDispatcher.addCallback(viewLifecycleOwner, childFragmentOnBackPressedCallback)
         childFragmentManager.addOnBackStackChangedListener(childBackStackListener)

* This function returns false if any of the mentioned system animations are disabled (0f),
* which addresses safe display mode and accessibility concerns.
*
* ANIMATION_DURATION_SCALE - controls app switching animation speed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the docs, apparently checking only this one is necessary.

https://developer.android.com/reference/android/provider/Settings.Global#TRANSITION_ANIMATION_SCALE

Scaling factor for Animator-based animations. This affects both the start delay and duration of all such animations. The value is a float. Setting to 0.0f will cause animations to end immediately. The default value is 1.0f.

On my phone (Android 16, Samsung), those scales can be changed only in the phone developer options. The public user facing setting only allows reducing animations in general.

You don't need to take any action based on this comment, but it's something that I think it's worth noting so the method may be simplified or better documented later.

Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meant to request changes

@BrayanDSO BrayanDSO added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Nov 19, 2025
@sanjaysargam sanjaysargam added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Nov 19, 2025
@BrayanDSO BrayanDSO added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Nov 19, 2025
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question on animations

Feel free to ignore the nitpick, unless something quick can be done

result.highlight(fragment as PreferenceFragmentCompat)
if (fragment is ControlsSettingsFragment) {
fragment.lifecycleScope.launch {
if (areSystemAnimationsEnabled(requireContext())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this only check system animations, ignoring the in-app 'safe display mode'

/**
* Whether animations should not be displayed
* This is used to improve the UX for e-ink devices
* Can be tested via Settings - Advanced - Safe display mode
*
* @see .animationEnabled
*/
fun animationDisabled(): Boolean {
val preferences = this.sharedPrefs()
return preferences.getBoolean("safeDisplay", false)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I tested with system animations disabled, the tab selection and preference highlighting don't work in the 'Animations disabled - do everything immediately' case. I think we need delays regardless of the
areSystemAnimationsEnabled condition

Copy link
Member

@david-allison david-allison Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this sat, could you provide reproduction steps or a video

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Nov 19, 2025
@sanjaysargam sanjaysargam added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Nov 19, 2025
@david-allison david-allison added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Nov 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Index control preferences in the settings search bar

4 participants